Per discussion above, the current rolling aggregation returns null under following corner cases (assume count is the number of valid observations in the window):

• count == 0
• count < min_periods
• ddof > count (results in negative variance) [EDIT 08/30/2021] - now valid but NaN, because it has valid input but mathematically ill defined

When ddof == count, the corresponding row is valid, the result is div by zero values (inf/nan).

FYI: In Spark, there is an option allowing you to specify whether to output a null or a NaN when divide-by-zero: https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/CentralMomentAgg.scala#L67

@ttnghia Is it a global switch or specific to std/var aggregation?

It's just specific to std/var aggregation. I'm not sure if this option is exposed to the user though. @revans2 do you have any idea?

It's just specific to std/var aggregation. I'm not sure if this option is exposed to the user though. @revans2 do you have any idea?

It is a global config in newer versions of Spark spark.sql.legacy.statisticalAggregate. It was added as a part of Spark 3.1.0 apparently as a bug fix to make the Spark SQL more ANSI complaint https://issues.apache.org/jira/browse/SPARK-13860. But like I said before we don't have to push this down onto CUDF to support. If CUDF wants to support it being configurable, that is great. But we can do a cheap non-null count aggregation on the same column and then swap out any values based on 0 or 1 counts to fix up the CUDF results to match Spark as needed.

@revans2 Thanks for the info. As mentioned several times by Bobby, for var/std case, doing a COUNT_VALID rolling aggregation on the input column can resolve the ambiguity if desired. Per discussion above, configurable null/nan has greater impact to the aggregation API as a whole, and is out of scope of this PR.

This appears to be just a copy of the GPU code for which it is testing but in CPU code. So a changes to one would likely require changing the other. The expected output is guaranteed to match the results if the test code logic always matches regardless if the algorithm is correct.

In defense of @isVoid's PR, the oldest rolling-window code was this way for a bit. I've tried moving to using more explicit (manually verifiable) values for the more recent feature additions. It will be worthwhile to dismantle the old tests and write explicit ones for them. As @davidwendt, @isVoid, etc. have noted, we should probably do that in a follow-up.

Regarding the change in behaviour since my last review (i.e. specifically when count < ddof: My understanding of Spark's stddev function is that it's equivalent to having ddof=1. The proposed behaviour doesn't look like it messes things up for Spark. Still 👍.

rerun tests

@gpucibot merge